Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use clang-format for code formatting #1598

Closed
wants to merge 1 commit into from

Conversation

zamazan4ik
Copy link
Contributor

For fixing I used clang-format -i -style=Google **/.h && clang-format -i -style=Google **/.cpp

clang-format 6.0.0

In this PR I suggest to discuss about code-style for Tesseract. Because FMPOV Google style isn't ideal for Tesseract and we can add some Tesseract-specific options.

For fixing I used clang-format -i -style=Google **/*.h && clang-format -i -style=Google **/*.cpp

clang-format 6.0.0
@amitdo
Copy link
Collaborator

amitdo commented May 23, 2018

#848 (comment)
#854

@stweil
Copy link
Member

stweil commented May 24, 2018

@Shreeshrii
Copy link
Collaborator

Shreeshrii commented May 24, 2018

#1598 is using clang-format 6.0.0

https://github.com/stweil/tesseract/tree/format is formatted using
clang-format-7

@stweil
Copy link
Member

stweil commented May 24, 2018

I now created https://github.com/stweil/tesseract/tree/clang-format-6 with clang-format 6.0.0-3. The result is identical to that from clang-format 7.0.0 (that means not the same as https://github.com/ZaMaZaN4iK/tesseract/tree/fix_format). It needs three iterations to stabilize. The formatting was done on Debian Buster.

@stweil
Copy link
Member

stweil commented May 24, 2018

@amitdo
Copy link
Collaborator

amitdo commented May 24, 2018

Run with clang 5 and 6/7:

clang-format -style=google -dump-config > .clang-format

Do you get the same result?

@amitdo
Copy link
Collaborator

amitdo commented May 24, 2018

Appveyor failed.
It doesn't like the openmp formatting in lstm.cpp.

@stweil
Copy link
Member

stweil commented May 24, 2018

Appveyor fails in my four tests, too. stweil@4ee0fdc shows the dumped configurations.

@stweil
Copy link
Member

stweil commented May 24, 2018

The Google style does not say whether type *variable or type* variable should be used. It only says that there should not be both variants in the same file. I suggest to use the same variant everywhere. Personally I prefer the first one, but using only the second one would also be fine. What do you think?

@amitdo
Copy link
Collaborator

amitdo commented May 24, 2018

I suggest to use the same variant everywhere.

👍

type *variable vs. type* variable

Personally I prefer the first one, but using only the second one would also be fine. What do you think?

It seems that Ray prefers type* variable (it's also my favorite style).
c1c1e42#diff-f5c88d7e2ff4fc9424ae517ce7b4e1a6

@zamazan4ik
Copy link
Contributor Author

@amitdo I also prefer type* variable

@zamazan4ik
Copy link
Contributor Author

I think we should dump existing one format profile (suggest use Google code style), correct it a little bit and push to Tesseract repo

@stweil
Copy link
Member

stweil commented May 24, 2018

https://github.com/stweil/tesseract/tree/format now uses uniform left aligned pointers and also includes a .clang-format file. Still missing: build fix for Appveyor.

@stweil
Copy link
Member

stweil commented Jun 20, 2018

We still have to decide when to do the formatting with clang-format. Should we do it before or after 4.0.0?

@zdenop
Copy link
Contributor

zdenop commented Jun 20, 2018

If yes, than we should do it before 4.0 release.

@johnson73A
Copy link

it is good

@zdenop
Copy link
Contributor

zdenop commented Sep 24, 2018

@zamazan4ik , @stweil : Can be this PR fixed / replaced / closed this week?

@stweil
Copy link
Member

stweil commented Sep 24, 2018

Yes, it can be closed. I think we won't reformat the whole code for 4.0.0.

@zdenop zdenop closed this Sep 24, 2018
@amitdo
Copy link
Collaborator

amitdo commented Oct 23, 2019

I suggest to re-format the whole codebase for 5.0.0.

@amitdo
Copy link
Collaborator

amitdo commented Oct 29, 2019

First step - reformat all headers in 'include/tesseract'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants